-
Notifications
You must be signed in to change notification settings - Fork 98
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
DO NOT MERGE | POC | Enhancement / 652 Migration UI From Old Modules to Standalone Plugins #806
DO NOT MERGE | POC | Enhancement / 652 Migration UI From Old Modules to Standalone Plugins #806
Conversation
…es-to-standalone-plugins
…ation-ui-from-old-modules-to-standalone-plugins
… plugin managment UI and REST endpoints.
…in applicable load files, enqueue necessary scripts for settings screen.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@10upsimon I left a bit of early feedback, mostly related to the overall approach outlined on #652.
On another note, please create a feature branch for this work, and then this PR should be based on that instead of trunk
.
// Load Composer dependencies if applicable. | ||
if ( file_exists( PERFLAB_PLUGIN_DIR_PATH . '/vendor/autoload.php' ) ) { | ||
require_once PERFLAB_PLUGIN_DIR_PATH . '/vendor/autoload.php'; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While Composer is generally great, using it in a WordPress plugin comes with additional quirks and interoperability problems that I would prefer to avoid unless truly beneficial. Let's require_once
any class files instead, as there shouldn't be any meaningful performance benefit from autoloading those few files anyway.
* @access private | ||
* @ignore | ||
*/ | ||
class REST_Plugins_Controller { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really need those REST routes? While I agree the REST API is generally the recommended approach over the oldschool AJAX request, since we are working on a WP core feature plugin here and since this functionality is supplemental, we should keep our maintenance burden as small as possible and use what core already provides. Is there a good reason not to use wp_ajax_install_plugin()
and WP_REST_Plugins_Controller
?
@@ -48,5 +48,8 @@ | |||
"psr-4": { | |||
"PerformanceLab\\Tests\\": "tests/utils" | |||
} | |||
}, | |||
"autoload": { | |||
"classmap": ["includes/"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See above, let's avoid the usage of Composer within the actual plugin.
$managed_standalone_plugins = array( | ||
'webp-uploads' => array( | ||
'Name' => esc_html__( 'WebP Uploads', 'performance-lab' ), | ||
'Description' => esc_html__( 'This plugin adds WebP support for media uploads within the WordPress application.', 'performance-lab' ), | ||
'Author' => esc_html__( 'WordPress Performance Team', 'performance-lab' ), | ||
'AuthorURI' => esc_url( 'https://make.wordpress.org/performance/' ), | ||
'PluginURI' => esc_url( 'https://wordpress.org/plugins/webp-uploads/' ), | ||
'Download' => 'wporg', | ||
), | ||
'sqlite-database-integration' => array( | ||
'Name' => esc_html__( 'SQLite Database Integration', 'performance-lab' ), | ||
'Description' => esc_html__( 'Allows testing an SQLite integration with WordPress and gather feedback, with the goal of eventually landing it in WordPress core.', 'performance-lab' ), | ||
'Author' => esc_html__( 'WordPress Performance Team', 'performance-lab' ), | ||
'AuthorURI' => esc_url( 'https://make.wordpress.org/performance/' ), | ||
'PluginURI' => esc_url( 'https://wordpress.org/plugins/sqlite-database-integration/' ), | ||
'Download' => 'wporg', | ||
), | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All this shouldn't be hard-coded. Instead, we should rely on the plugin.json
file for the slugs of which plugins to cover here, and all other data should come from the WordPress.org API, which we can query in the same way that it's done on the Plugins > Add New screen.
|
||
$managed_standalone_plugins[ $plugin_slug ]['Status'] = $status; | ||
$managed_standalone_plugins[ $plugin_slug ]['Slug'] = $plugin_slug; | ||
$managed_standalone_plugins[ $plugin_slug ]['HandoffLink'] = isset( $managed_standalone_plugins[ $plugin_slug ]['EditPath'] ) ? admin_url( $managed_standalone_plugins[ $plugin_slug ]['EditPath'] ) : null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does "HandoffLink" refer to? What is it for?
return new WP_Error( 'wpp_plugin_failed_deactivation', __( 'Failed to deactivate plugin.', 'performance-lab' ) ); | ||
} | ||
return true; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similarly to my comment on the REST classes, why do we need methods to activate and deactivate? Can't we use what core already provides for that purpose?
Summary
Addresses: #652
Relevant technical choices
Checklist
[Focus]
orInfrastructure
label.[Type]
label.no milestone
label.